Conversation
Chart.svelte checked isFetching before the query error, so a failed fill-missing aggregation never left the diamond.
nishantmonu51
added
Type:Bug
|
1. The query client caps retries ( 2. The tests do not cover the scenario that produced the bug. Every case passes renderChart(chartData({ isFetching: false, error: new Error("...") }), []);Also, 3. The
4. Minor: a stale error now suppresses the spinner on refetch. With |
nishantmonu51
left a comment
nishantmonu51
left a comment
There was a problem hiding this comment.
The branch reorder is correct and fixes the reported symptom. Two comments below, on what the new spec does not cover and on the unrelated vitest-setup.ts change.
| describe("Chart loading and error states", () => { | ||
| it("shows the query error instead of the spinner", () => { | ||
| const { container, queryByText } = renderChart( | ||
| chartData({ | ||
| isFetching: true, | ||
| error: new Error("time range has too many bins"), | ||
| }), | ||
| ); | ||
|
|
||
| expect(queryByText("time range has too many bins")).toBeInTheDocument(); | ||
| expect(container.querySelector(".status")).toBeNull(); | ||
| }); |
There was a problem hiding this comment.
Every case here passes measures = [measure], so the measures.length === 0 half of the loading branch, which is what actually kept the chart spinning, is never exercised. With a non-empty measures the old {:else if error} branch was already reachable: the query client only retries network errors (web-common/src/lib/svelte-query/globalQueryClient.ts:13-14), so a failed aggregation settles with isFetching: false and error set. The regression to pin is chartData({ isFetching: false, error }) with measures: [], which is the state of a chart pointed at a missing or invalid metrics view, since getMeasuresForMetricView returns [] whenever there is no validSpec (web-common/src/features/metrics-views/metrics-view-selectors.ts:100-105). The existing isFetching: true plus error case only covers refetch-after-failure.
| // Node 22+ exposes a global `localStorage` that is undefined unless | ||
| // --localstorage-file is set. Theme and other stores read the global at | ||
| // import time, so pin it to a memory-backed mock for jsdom tests. | ||
| if ( | ||
| typeof globalThis.localStorage === "undefined" || | ||
| typeof globalThis.localStorage?.getItem !== "function" | ||
| ) { | ||
| const memory = new Map<string, string>(); | ||
| const localStorageMock: Storage = { | ||
| get length() { | ||
| return memory.size; | ||
| }, | ||
| clear: () => memory.clear(), | ||
| getItem: (key: string) => memory.get(key) ?? null, | ||
| key: (index: number) => [...memory.keys()][index] ?? null, | ||
| removeItem: (key: string) => { | ||
| memory.delete(key); | ||
| }, | ||
| setItem: (key: string, value: string) => { | ||
| memory.set(key, value); | ||
| }, | ||
| }; | ||
| Object.defineProperty(globalThis, "localStorage", { | ||
| configurable: true, | ||
| writable: true, | ||
| value: localStorageMock, | ||
| }); | ||
| Object.defineProperty(window, "localStorage", { | ||
| configurable: true, | ||
| writable: true, | ||
| value: localStorageMock, | ||
| }); | ||
| } |
There was a problem hiding this comment.
This shim is unrelated to the branch reorder and is inert on the pinned toolchain: .nvmrc is Node 22, where localStorage exists only behind --experimental-webstorage, and Chart.spec.ts passes on Node 20 with this file reverted to main. It is needed only on Node 25+, where localStorage is an unflagged global that is undefined without --localstorage-file and ThemeControl reads it at import time via explicitLocalStorageStore (web-common/src/features/themes/theme-control.ts:22-24), so the comment's claim about Node 22+ is inaccurate as written. If it stays, note that clearMocks: true does not reset the backing Map, so storage written by one test is visible to the next in the same file.
This branch has not been deployed
2 participants
Chart.sveltecheckedisFetchingbefore the queryerror, so a failed request never left the loading state.Checklist:
Developed in collaboration with Claude Code